Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: conditionally enable regression methods based on term type and s… #2586

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

xzhou82
Copy link
Collaborator

@xzhou82 xzhou82 commented Jan 10, 2025

…how customized Regression chart button if a single method is available

Description

at termdbtest, it shows Regression Analysis button with 3 options
at mbmeta, it shows Cox Regression and directly launches ui
at allpharm, Cox option no longer shows

nav CI passes

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

…how customized Regression chart button if a single method is available
linear: ({ cohortTermTypes }) => {
return cohortTermTypes.numeric > 0 // numeric term present and could be used as linear outcome
},
logistic: () => true, // consider both numeric & categorical can be logistic outcome
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add 'binary' outcome

return cohortTermTypes.numeric > 0 // numeric term present and could be used as linear outcome
},
logistic: () => true, // consider both numeric & categorical can be logistic outcome
cox: ({ cohortTermTypes }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add example, 'time to event'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what u mean

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add examples for comments I meant:
I can add them in if you'd like.

linear: ({ cohortTermTypes }) => {
    // Check if there is at least one numeric term present, which can be used as a linear outcome
    // Example: outcome is a continuous variable.
    return cohortTermTypes.numeric > 0;
},
logistic: () => true, 
    // Allow both numeric and categorical terms to be considered as logistic outcomes
    // Example: Predicting whether a patient has a disease (yes/no), where the outcome is a binary variable
},
cox: ({ cohortTermTypes }) => {
    // Ensure there is at least one survival or condition term present, which is required for a Cox outcome
    // Example: Analyzing the time until a patient relapses after treatment, where the outcome is the time until the event occurs
    return (cohortTermTypes.survival || 0) + (cohortTermTypes.condition || 0) > 0;
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

survival and condition terms are time-to-event variables and can be used as Cox outcome. these are facts outside of our code thus not really explained

if (availableMethods.length > 1) return obj // more than 1 regression methods. do not modify original obj
if (availableMethods.length == 1) {
// has only one method. customized button label and click behavior
const m = availableMethods[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel in our codebase we should refrain from using alphabets like 'm' to assign values. I understand that it's within the scope of that block but it's in several places in the codebase and searching becomes difficult if used in several scripts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making an improvement for this and backend

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done please check again

Copy link
Contributor

@karishma-gangwani karishma-gangwani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@xzhou82 xzhou82 merged commit e9f66b9 into master Jan 10, 2025
3 checks passed
@xzhou82 xzhou82 deleted the issue.2582 branch January 10, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants